Skip to content

fix: remove 300ms click delay by setting click interval to 0#10388

Open
cschanhniem wants to merge 1 commit into
visgl:masterfrom
cschanhniem:fix/click-delay-interval-0
Open

fix: remove 300ms click delay by setting click interval to 0#10388
cschanhniem wants to merge 1 commit into
visgl:masterfrom
cschanhniem:fix/click-delay-interval-0

Conversation

@cschanhniem

Copy link
Copy Markdown

PR #10323 corrected the requestFailurerequireFailure typo in the recognizer setup, which activated a 300ms delay on every click via TapRecognizer's unconditional setTimeout when requireFailure is set.

Setting interval: 0 on the click recognizer makes the timer fire immediately, restoring the sub-millisecond click response that existed in all versions prior to 9.3.4.

Fixes #10384

PR visgl#10323 corrected the `requestFailure` → `requireFailure` typo in the
recognizer setup, which activated a 300ms delay on every click via
TapRecognizer's unconditional setTimeout when `requireFailure` is set.

Setting `interval: 0` on the click recognizer makes the timer fire
immediately, restoring the sub-millisecond click response that existed
in all versions prior to 9.3.4.

Fixes visgl#10384

Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 83.428% (+0.004%) from 83.424% — cschanhniem:fix/click-delay-interval-0 into visgl:master

@charlieforward9

Copy link
Copy Markdown
Collaborator

To avoid more issues with this gesture handler, adding some tests would be helpful.

That is, unless short term it is preferred to move these handlers into mjolnir.js

thoughts? @chrisgervang

@chrisgervang chrisgervang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can merge this to hot fix but since we're also the maintainers of mjolnir, I would like to get more context around why 300ms is the default and potentially change it to avoid this class of bug elsewhere.

cc @Pessimistress @ibgreen

@cschanhniem

Copy link
Copy Markdown
Author

Good call — I will add tests for the gesture handler to prevent regressions. Short term I can land those in this PR; happy to move them into mjolnir.js if thats the preferred home.

@cschanhniem

Copy link
Copy Markdown
Author

Good point — I'll follow up by looking at mjolnir.js to understand why the TapRecognizer timer defaults to 300ms. If there's no semantic reason beyond 'tap is a delayed click,' we could upstream a shorter interval there so the fix applies across all consumers.

@chrisgervang

Copy link
Copy Markdown
Collaborator

Sounds good. Please ping me after you've looked into tests to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] 9.3.4 regression: all clicks now have ~300ms delay due to requireFailure fix

4 participants